-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add streaming payments tutorial #2
Conversation
dcde0af
to
5f4b4e2
Compare
Updated for IL-RFC-4, draft 6. |
5f4b4e2
to
0c08c6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't got the time to review the JS in detail but the concept LGTM
streaming-payments.md
Outdated
|
||
## What you'll learn: | ||
|
||
* convert the proxy from the Letter Shop tutorial into a payment tool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "payment tool"?
streaming-payments.md
Outdated
* using the ILP packet | ||
* streaming payments | ||
* how to use trustlines to speed things up | ||
* Bilateral Transfer Protocol (BTP) and its relation to ILP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a lot to include in one tutorial
streaming-payments.md
Outdated
res._headerSent = true | ||
}).listen(8000) | ||
}) | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think small code samples next to explanations of what's going on are more helpful than this kind of large code block. If someone wants to see the whole thing at once they should just go to the included source file
streaming-payments.md
Outdated
|
||
## The Pay Header | ||
|
||
We'll change the Letter Shop from the previous tutorial a bit, to `shop2.js`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth a small mention of what we're trying to change this to do
streaming-payments.md
Outdated
const parts = ilpPacketContents.account.split('.') | ||
// 0: test, 1: crypto, 2: xrp, 3: rrhnXcox5bEmZfJCHzPxajUtwdt772zrCW, 4: userId, 5: paymentId | ||
if (parts.length < 6 || typeof users[parts[4]] === 'undefined' || ilpPacketContents.amount !== transfer.amount) { | ||
plugin.rejectIncomingTransfer(transfer.id, {}).catch(function () {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a bit of a janky way to deal with the ilp address. Also, if you're rejecting the transfer it would be better to put some kind of message as to why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
janky way to deal with the ilp address
You mean appending the userId to the destination address? Isn't this what SPSP does as well, with the receiver id and token?
streaming-payments.md
Outdated
node ./shop3.js | ||
``` | ||
|
||
And here is the content consumption tool, which first transfers 1000 XRP to its account at the shop's ledger, and then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think someone might reasonably be confused about why it's better to transfer all the money to the shop before they send you anything than to do it in exchange for the letters. I think BTP makes more sense when you're funding an account with a connector that you'll use for a lot of payments to different receivers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, actually that text is incorrect, that's what I wanted to do first, but I ended up just sending the drops from a setInterval
. But that's still putting a lot of trust in the shop; I could change the code so that it counts how many letters were received, and limits the in-flight amount (e.g. if the shop is behind by more than 10 letters, throttle the money sending)
streaming-payments.md
Outdated
consumes 1000 letters. Save it as `tool2.js`: | ||
```js | ||
const IlpPacket = require('ilp-packet') | ||
const Plugin = require('ilp-plugin-payment-channel-framework') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be more impressive to show how the sender switching to BTP makes their payments faster without the receiver having to change anything (or vice versa)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but for that we need to connect to Amundsen - I was saving that for the third tutorial. This is still just a one-transfer payment, so shop and customer still need to be on the same ledger.
inRes.body.pipe(process.stdout) | ||
const payHeaderParts = inRes.headers.get('Pay').split(' ') | ||
console.log(payHeaderParts) | ||
// e.g. Pay: 1 test.crypto.xrp.asdfaqefq3f.26wrgevaew SkTcFTZCBKgP6A6QOUVcwWCCgYIP4rJPHlIzreavHdU |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be a different address now
streaming-payments.md
Outdated
We used the ILP packet for the first time, talked about connectors, payments as chains of transfers, and used | ||
the `Pay` header, so that the shop could request payment from the tool that connects to it. | ||
|
||
We added a BTP-enabled ledger to the shop, so that our content consumption tool can receive letters faster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is too much to introduce in one tutorial. I think it would be better to separate trustlines/BTP from the Pay
header
shop3.js
Outdated
_store: { | ||
get: (k) => store[k], | ||
put: (k, v) => { store[k] = v }, | ||
del: (k) => delete store[k] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth noting in a comment that it wouldn't be a good idea to use an in-memory store like this in practice but this is just for the example
bb716b8
to
4d06a8c
Compare
2cc3856
to
5ef84cf
Compare
Even though @emschwartz did a previous review of this PR, requested review from @sharafian this time, because 1) he worked on http-ilp, and 2) he is working on developer onboarding. |
comments addressed (except maybe #2 (comment) as the section that describes BTP is still a bit dense; will continue working on that with Ben)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as a first version; I'll submit a PR to update to our latest tools
OK, merging this so we can improve it further |
* feat: add streaming payments tutorial * use psk * use psk * update streaming payments tutorial * list trustlines tutorial * fix: improve paragraph about BTP symmetry
No description provided.